LibWeb: Move CSS value evaluation onto the Rust value graph - #10861

Tests/LibWeb/TestStyleValueEquality.cpp (1)

1308-1316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a helper for the repeated FfiAnimationContext literal.

The same 7-field initializer is duplicated at Lines 1333-1341, 1379-1387, 1428-1436, 2090-2098, 2389-2397 and 2441-2449. Since this FFI struct is actively changing in this migration, every added field means touching seven test sites.

♻️ Suggested helper

static StyleValueFFI::FfiAnimationContext make_animation_context(bool allow_discrete)
{
    return StyleValueFFI::FfiAnimationContext {
        .allow_discrete = allow_discrete,
        .current_color = nullptr,
        .has_length_resolution_context = false,
        .length_resolution_context = {},
        .has_transform_reference_box = false,
        .transform_reference_box_width = 0,
        .transform_reference_box_height = 0,
    };
}

Call sites then become auto context = make_animation_context(false);, with the reference-box case overriding the two fields it needs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Tests/LibWeb/TestStyleValueEquality.cpp` around lines 1308 - 1316, Extract a
shared make_animation_context(bool allow_discrete) helper for the repeated
StyleValueFFI::FfiAnimationContext initialization, preserving the existing
default field values and passing allow_discrete through. Replace all duplicated
literals in the affected tests with this helper, while retaining any
reference-box-specific field overrides at their call sites.

Libraries/LibWeb/CSS/StyleValues/CalculatedStyleValue.cpp (1)

132-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider named constants for the FFI piece/node kind codes.

piece.kind, piece.numeric_kind, and node.kind are matched as bare integers here (and again in CalcNodeRef::numeric), so the Rust discriminants are only pinned by convention. Mirroring them as small enum class values next to the existing static_assert block would make drift a compile error instead of a VERIFY_NOT_REACHED() at runtime.

Also applies to: 499-579

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/CalculatedStyleValue.cpp` around lines 132 -
182, Define named enum class values for the FFI piece, numeric, and node kind
discriminants beside the existing static_assert block, preserving the Rust
numeric values. Update the switches in calculated style serialization and
CalcNodeRef::numeric to use these named constants instead of bare integer
literals, including piece.kind and piece.numeric_kind, so mismatches are caught
at compile time.

Libraries/LibWeb/CSS/Rust/src/calc.rs (2)

3526-3535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document that the returned pieces borrow the calculation's style values.

CalcSerializer::style_value stores the raw StyleValueData backing pointer without retaining it, so every FfiCalcSerialization piece is only valid while calculated (and hence its calculation tree) stays alive. Worth stating in the safety comment next to the existing calculated requirement so a future caller doesn't release the value before draining the batch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/calc.rs` around lines 3526 - 3535, Update the
safety documentation for rust_calc_serialize to state that returned
FfiCalcSerialization pieces borrow style values from calculated and remain valid
only while calculated and its calculation tree stay alive; retain the existing
requirement that calculated points to valid Calculated style value data.

---

3842-3892: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Bail out of the reification walk as soon as a child fails.

for_each_child keeps recursing after failed is set, so an unsupported node deep in a large tree still walks and pushes entries for every remaining sibling subtree before the caller discards everything. A short-circuit keeps the common failure path cheap.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/calc.rs` around lines 3842 - 3892, Update the
child traversal in append_reification_node so its for_each_child callback
returns immediately once failed is true, avoiding recursion into remaining
sibling subtrees after append_reification_node returns None. Preserve the
existing failed state and final None result for unsupported descendants.

Libraries/LibWeb/CSS/StyleComputer.cpp (1)

1765-1793: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Assert the values each action kind requires before dereferencing them.

Start/RemoveAndStart dereference before_change_value/after_change_value, which are only populated when has_matching_transition == Yes, and the reversing/interrupted kinds dereference current_value, only populated when has_running_transition. If the Rust decision ever emits a kind outside those preconditions this is a null deref rather than a clear failure. A VERIFY per branch documents and enforces the FFI contract cheaply.

🛡️ Proposed guards

         case StyleValueFFI::FfiTransitionActionKind::Start:
+            VERIFY(prepared_transition.before_change_value && prepared_transition.after_change_value);
             start_a_transition(*prepared_transition.before_change_value, *prepared_transition.after_change_value, *prepared_transition.before_change_value);
             break;
         case StyleValueFFI::FfiTransitionActionKind::RemoveAndStart:
+            VERIFY(prepared_transition.before_change_value && prepared_transition.after_change_value);
             remove_existing_transition();
             start_a_transition(*prepared_transition.before_change_value, *prepared_transition.after_change_value, *prepared_transition.before_change_value);
             break;
         case StyleValueFFI::FfiTransitionActionKind::CancelRemoveAndStartReversing: {
             VERIFY(existing_transition);
+            VERIFY(prepared_transition.current_value && prepared_transition.after_change_value);
             auto reversing_adjusted_start_value = existing_transition->transition_end_value();
             cancel_and_remove_existing_transition();
             start_a_transition(*prepared_transition.current_value, *prepared_transition.after_change_value, *reversing_adjusted_start_value);
             break;
         }
         case StyleValueFFI::FfiTransitionActionKind::CancelRemoveAndStartInterrupted:
+            VERIFY(prepared_transition.current_value && prepared_transition.after_change_value);
             cancel_and_remove_existing_transition();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleComputer.cpp` around lines 1765 - 1793, Assert each
transition action’s required prepared values before dereferencing them in the
switch. Add VERIFY checks for before_change_value and after_change_value in
Start and RemoveAndStart, and for current_value in CancelRemoveAndStartReversing
and CancelRemoveAndStartInterrupted; preserve the existing transition operations
and reversing_adjusted_start_value handling.

Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.h (1)

49-51: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Return the stored components by const reference.

first_component()/second_component() copy a ColorMixComponent (two refcount bumps) per call, and serialize alone calls them four times (Lines 312-316 of ColorMixStyleValue.cpp). Now that they are plain members, ColorMixComponent const& is free.

♻️ Proposed refactor

-    ColorMixComponent first_component() const { return m_first_component; }
-    ColorMixComponent second_component() const { return m_second_component; }
+    ColorMixComponent const& first_component() const { return m_first_component; }
+    ColorMixComponent const& second_component() const { return m_second_component; }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.h` around lines 49 - 51,
Update ColorMixStyleValue::first_component() and second_component() to return
ColorMixComponent const& instead of values, preserving their existing access to
m_first_component and m_second_component; leave
color_interpolation_method_value() unchanged.

Libraries/LibWeb/CSS/StyleValues/ColorFunctionStyleValue.h (1)

38-41: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider returning channels() by const reference.

channels() copies three ValueComparingNonnullRefPtr (three atomic increments/decrements) per call, and it's called repeatedly per invocation in to_color, absolutized, and serialize (e.g. Lines 506-510 and 521-528 of Libraries/LibWeb/CSS/StyleValues/ColorFunctionStyleValue.cpp). Since the array is now a stable member, a const reference is free.

♻️ Proposed refactor

-    Array<ValueComparingNonnullRefPtr<StyleValue const>, 3> channels() const
+    Array<ValueComparingNonnullRefPtr<StyleValue const>, 3> const& channels() const
     {
         return m_channels;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/ColorFunctionStyleValue.h` around lines 38 -
41, Update ColorFunctionStyleValue::channels() to return a const reference to
the stable m_channels member instead of returning the array by value, preserving
its const access and existing callers.

Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.cpp (1)

391-401: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Unconditional default-method allocation on every to_color() call.

ColorInterpolationMethodStyleValue::create(RectangularColorSpace::Oklab) allocates a C++ shell plus a Rust StyleValueData on every call, even when color_interpolation_method_value() is present. Consider a function-local static (the value is immutable and shareable) or constructing it only in the null branch. Same pattern at Lines 422-425.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.cpp` around lines 391 -
401, Update the default interpolation-method setup in to_color() so
ColorInterpolationMethodStyleValue::create(RectangularColorSpace::Oklab) runs
only when color_interpolation_method_value() is absent, avoiding allocation when
an explicit method exists. Apply the same lazy-default change to the
corresponding setup around the second occurrence at lines 422-425, while
preserving the existing Oklab fallback behavior.

Libraries/LibWeb/CSS/StyleValues/BorderImageSliceStyleValue.h (1)

36-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract shared helpers for the repeated retain/adopt/create idiom.

Every migrated subclass repeats the same two one-liners verbatim: "retain-then-create" (StyleValueFFI::rust_style_value_retain(x->rust_style_value_data()) fed into a rust_style_value_create_* call) and "retain-then-adopt" (StyleValue::adopt_rust_style_value_data(StyleValueFFI::rust_style_value_retain(ptr))). BorderImageSliceStyleValue.h alone repeats this 8 times; the same shape recurs across the whole cohort.

• Libraries/LibWeb/CSS/StyleValues/BorderImageSliceStyleValue.h#L36-L52: replace the 4 retain+adopt calls in the FFI-data ctor and 4 retain+create calls in the value ctor with two small StyleValue static helpers (e.g. StyleValue::adopt_retained_child(ptr) and StyleValue::retained_data(value)).
• Libraries/LibWeb/CSS/StyleValues/BackgroundSizeStyleValue.cpp#L15-L17: use the "retain-then-create" helper for size_x/size_y.
• Libraries/LibWeb/CSS/StyleValues/BackgroundSizeStyleValue.h#L38-L48: use the "retain-then-adopt" helper for size_x/size_y.
• Libraries/LibWeb/CSS/StyleValues/BorderRadiusRectStyleValue.h#L48-L64: use both helpers for the 4 corners.
• Libraries/LibWeb/CSS/StyleValues/BorderRadiusStyleValue.h#L45-L57: use both helpers for horizontal/vertical radius.
• Libraries/LibWeb/CSS/StyleValues/CounterStyleSystemStyleValue.h#L76-L98: use both helpers for the optional first_symbol.
• Libraries/LibWeb/CSS/StyleValues/PendingSubstitutionStyleValue.h#L44-L52: use both helpers for original_shorthand_value.
• Libraries/LibWeb/CSS/StyleValues/CounterDefinitionsStyleValue.h#L53-L62: use the "retain-then-create" helper per-entry in the loop.

A single point of truth for this retain-count bump reduces the risk of a future subclass forgetting the retain (leading to a use-after-free) or double-retaining (a leak).

♻️ Example helper shape

// In StyleValue.h (private/protected static helpers)
static ValueComparingNonnullRefPtr<StyleValue const> adopt_retained_child(StyleValueFFI::StyleValueData const* pointer)
{
    return adopt_rust_style_value_data(StyleValueFFI::rust_style_value_retain(pointer));
}

static StyleValueFFI::StyleValueData const* retained_data(StyleValue const& value)
{
    return StyleValueFFI::rust_style_value_retain(value.rust_style_value_data());
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/BorderImageSliceStyleValue.h` around lines
36 - 52, The repeated retain/adopt and retain/create expressions should be
centralized in two StyleValue static helpers that each perform exactly one
retain-count bump. Add helpers such as adopt_retained_child and retained_data,
then update Libraries/LibWeb/CSS/StyleValues/BorderImageSliceStyleValue.h lines
36-52 to use them for all four children in both constructors; apply the
corresponding helper at
Libraries/LibWeb/CSS/StyleValues/BackgroundSizeStyleValue.cpp lines 15-17,
BackgroundSizeStyleValue.h lines 38-48, BorderRadiusRectStyleValue.h lines
48-64, BorderRadiusStyleValue.h lines 45-57, CounterStyleSystemStyleValue.h
lines 76-98, PendingSubstitutionStyleValue.h lines 44-52, and
CounterDefinitionsStyleValue.h lines 53-62, including each loop entry and
optional first_symbol without changing ownership semantics.

Libraries/LibWeb/CSS/StyleValues/TransformationStyleValue.h (1)

29-32: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider returning m_values by const reference.

values() now copies the cached StyleValueVector (a Vector of ref-counted pointers) on every call, incurring a refcount bump per element. Since m_values is already a stable member, returning StyleValueVector const& would avoid the copy for callers (e.g. ShorthandStyleValue::serialize) that only read from it. This is already a big improvement over the previous per-call Rust rematerialization, so the remaining copy cost is low.

Optional tweak

-    StyleValueVector values() const
+    StyleValueVector const& values() const
     {
         return m_values;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/TransformationStyleValue.h` around lines 29
- 32, Update the values() accessor to return a const reference to the existing
m_values member instead of returning StyleValueVector by value, preserving
read-only access while avoiding per-call vector and refcount copies.

Libraries/LibWeb/CSS/Rust/src/animation.rs (6)

6346-6352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Parenthesize the mixed ||/&& fallback condition.

!result.handled || result.value.is_null() && context.is_some_and(...) relies on && binding tighter than ||. The behavior is correct (handled-with-null and allow_discrete false must return the null result), but this is the file's most consequential branch and reads as if it were (!handled || null) && allow_discrete. Explicit parentheses cost nothing.

♻️ Proposed change

-    if !result.handled || result.value.is_null() && context.is_some_and(|context| context.allow_discrete) {
+    if !result.handled || (result.value.is_null() && context.is_some_and(|context| context.allow_discrete)) {
         return discrete_value(context, from, to, delta);
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/animation.rs` around lines 6346 - 6352,
Parenthesize the `result.value.is_null() && context.is_some_and(|context|
context.allow_discrete)` portion of the fallback condition in the interpolation
flow, preserving the existing `!result.handled || (...)` behavior and return
paths.

---

693-699: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Integer interpolation loses precision above 2^24.

interpolate_i32 does the arithmetic in f32, so large operands (e.g. z-index: 16777217) round to the wrong integer even at delta 0 or 1. The surrounding code already works in f64; using it here removes the artifact at no cost.

♻️ Proposed change

-    let value = (from as f32 + (to as f32 - from as f32) * delta).round();
-    clamp_to_range(f64::from(value), range) as i32
+    let value = (f64::from(from) + (f64::from(to) - f64::from(from)) * f64::from(delta)).round();
+    clamp_to_range(value, range) as i32

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/animation.rs` around lines 693 - 699, Update
interpolate_i32 to perform interpolation arithmetic in f64 rather than f32,
including the conversion of from, to, and delta before rounding. Preserve the
existing clamp_to_range behavior and i32 return conversion so integer endpoints
such as values above 2^24 remain exact at delta 0 and 1.

---

4089-4108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated quaternion slerp.

The inline slerp inside interpolate_rotate_3d is a line-for-line copy of slerp_quaternions (Line 4693), including the f32::EPSILON degeneracy checks. Two copies of numerically delicate code will drift; call the helper instead.

Also applies to: 4693-4713

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/animation.rs` around lines 4089 - 4108, The
inline quaternion slerp in interpolate_rotate_3d duplicates the existing
slerp_quaternions implementation. Replace the local product, angle, weight, and
degeneracy-handling logic with a call to slerp_quaternions, passing the same
from_quaternion, to_quaternion, and delta inputs, while preserving the current
interpolation result.

---

6567-6570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit coverage for the pure-math transform paths.

The module's most intricate logic — decompose_matrix/recompose_matrix, interpolate_matrices, slerp_quaternions, interpolate_rotate_3d, and the grid track expansion — has no unit tests here and is only exercised indirectly through the WPT text expectations. A round-trip assertion (recompose_matrix(decompose_matrix(m)) ≈ m) and a degenerate-axis rotate3d case would have caught the NaN path flagged at Line 4064 directly, with far tighter feedback than a WPT diff.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/animation.rs` around lines 6567 - 6570, Add
unit tests in the existing tests module covering the pure-math paths
decompose_matrix/recompose_matrix, interpolate_matrices, slerp_quaternions,
interpolate_rotate_3d, and grid track expansion. Include a matrix round-trip
assertion with approximate equality and a degenerate-axis rotate3d case
verifying finite, expected behavior, so the NaN path is caught directly.

---

6079-6110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Collapse the repeated ANIMATION_TYPE_CUSTOM && chain.

animation_type == ANIMATION_TYPE_CUSTOM is re-tested about a dozen times, each paired with a property-id comparison, before the final catch-all discrete fallback. A single if animation_type == ANIMATION_TYPE_CUSTOM { match property_id { ... } } would express "custom algorithms, dispatched by property" directly and make it obvious that every custom property either returns or falls through to discrete.

Also applies to: 6184-6186, 6245-6292, 6306-6344

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/animation.rs` around lines 6079 - 6110,
Refactor the custom-animation handling in the surrounding interpolation function
to check animation_type == ANIMATION_TYPE_CUSTOM once, then dispatch
property-specific algorithms through a single match on property_id, including
the existing filter, shadow, stroke-dasharray, and other custom-property
branches referenced by the comment. Preserve each branch’s current return and
fall-through behavior so unhandled custom properties still reach the existing
discrete fallback.

---

26-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the generated enum constants instead of raw discriminants.
These values already come from the shared CSS enum source, so copying them into animation.rs adds a second source of truth.

• Libraries/LibWeb/CSS/Rust/src/animation.rs#L26-L86: import the generated css_enums constants instead of duplicating VALUE_TYPE_*, TRANSFORM_FUNCTION_*, COLOR_TYPE_*, STEP_POSITION_*, and BASIC_SHAPE_*.
• Libraries/LibWeb/CSS/Rust/src/animation.rs#L5593-L5597, #L5684-L5697, #L5749-L5757: replace the bare ColorFilterType ordinals with named constants as well.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/animation.rs` around lines 26 - 86, In
Libraries/LibWeb/CSS/Rust/src/animation.rs at lines 26-86, remove the duplicated
VALUE_TYPE_*, TRANSFORM_FUNCTION_*, COLOR_TYPE_*, STEP_POSITION_*, and
BASIC_SHAPE_* definitions and import the corresponding generated css_enums
constants. At lines 5593-5597, 5684-5697, and 5749-5757, replace bare
ColorFilterType ordinals with the generated named constants; update all
references to preserve the existing values and behavior.

Tests/LibWeb/Text/input/css/repeatable-list-animation-rust.html (1)

15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Counters are reset but never asserted.

resetStyleFfiCounters() has no effect on this test's output, so the -rust suffix isn't actually guarded — the test would still pass if evaluation fell back to a non-Rust path. Either drop the reset, or print internals.styleFfiCounters().animationEvaluationEntries the way transition-effect-batch-rust.html does (remember to update the expected .txt). The same unused reset appears in Tests/LibWeb/Text/input/css/shadow-animation-rust.html Line 12.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Tests/LibWeb/Text/input/css/repeatable-list-animation-rust.html` around lines
15 - 17, Update the Rust animation tests around resetStyleFfiCounters(),
including repeatable-list and shadow-animation cases, so the reset is either
removed or followed by output of
internals.styleFfiCounters().animationEvaluationEntries to verify Rust
evaluation; update the corresponding expected output files if counters are
printed.

Libraries/LibWeb/CSS/Rust/src/color_conversion.rs (4)

284-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated hue derivation between srgb_to_hwb and srgb_to_hsl.

Lines 291-301 are a copy of Lines 264-278 minus the negative-saturation fixup. Extracting a shared fn srgb_hue(red, green, blue, chroma) -> f32 would remove the drift risk between the two copies.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_conversion.rs` around lines 284 - 304,
Extract the duplicated hue calculation from srgb_to_hwb and srgb_to_hsl into a
shared srgb_hue(red, green, blue, chroma) helper returning f32. Update both
conversion functions to call this helper while preserving their existing
saturation-specific behavior and output values.

---

231-231: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent alpha handling: only hsl_to_srgb clamps alpha.

Every other conversion in this module passes color[3] through unchanged; this one clamps to [0, 1]. That makes convert()'s alpha behavior depend on the source space, which is surprising for a pure conversion table (and clamping is already the caller's job in color_interpolation.rs Line 324).

♻️ Proposed consistency fix

-    [convert(0.0), convert(8.0), convert(4.0), color[3].clamp(0.0, 1.0)]
+    [convert(0.0), convert(8.0), convert(4.0), color[3]]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_conversion.rs` at line 231, Update the
hsl_to_srgb conversion entry to pass color[3] through unchanged instead of
clamping it, matching the alpha handling of the other conversion functions and
leaving range clamping to the caller.

---

493-513: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing the reverse SRGB → RGB fast path.

Line 494 short-circuits RGB → SRGB but not the reverse, so SRGB → RGB falls through to Line 512 and round-trips through linear-light XYZ D65 — two transfer-function applications and two matrix multiplies that should be identity, leaving float drift in the result. Also worth parenthesizing the mixed ||/&& for readability.

♻️ Proposed fix

-    if color_type == target_type || color_type == RGB && target_type == SRGB {
+    if color_type == target_type
+        || (color_type == RGB && target_type == SRGB)
+        || (color_type == SRGB && target_type == RGB)
+    {
         return Some(color);
     }

Note this changes RGB's gamut-clamp behavior only for the SRGB → RGB direction, which to_xyz65 (Line 445) currently clamps and from_xyz65 (Line 472) does not — so confirm which side is meant to own clamping before applying.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_conversion.rs` around lines 493 - 513,
Update convert to short-circuit both RGB↔SRGB conversions, including the missing
SRGB-to-RGB path, while preserving the intended gamut-clamping behavior for that
direction; confirm whether clamping belongs on the source or destination side
before implementing. Parenthesize the mixed conditions in convert for
readability, especially the RGB/SRGB and HSL/HWB checks.

---

523-561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Round-trip test never exercises the piecewise linear segments.

All three components of source (0.25/0.5/0.75) sit above the knee of every transfer function, so the <= 16/512, < BETA, and <= 0.04045 branches — the parts most prone to a transposed constant — are never executed. Adding a near-zero sample would cover them, and RGB (legacy, the gamut-clamping arm) isn't in the list at all.

Also, Line 525 asserts float equality; the neighboring white assertion uses a tolerance, so applying one consistently would be less brittle.

💚 Proposed test extension

-        let source = [0.25, 0.5, 0.75, 0.8];
-        for color_type in [
+        for source in [[0.25, 0.5, 0.75, 0.8], [0.001, 0.01, 0.02, 1.0]] {
+        for color_type in [
             SRGB,

(then close the extra scope, and thread source through the existing assertions)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_conversion.rs` around lines 523 - 561,
Extend round_trips_supported_color_spaces to test a near-zero source sample that
exercises the low-end piecewise transfer-function branches, and include the
legacy RGB color space so its gamut-clamping path is covered. Thread each sample
through the existing conversion and component assertions, preserving the current
sample, and update converts_srgb_endpoints_to_oklab to compare the black
endpoint with the same tolerance-based approach used for white.

Libraries/LibWeb/CSS/Rust/src/color_interpolation.rs (3)

364-364: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Clamp alpha after applying alpha_multiplier.

interpolated_alpha is clamped at Line 324, but the multiplier is applied afterwards without re-clamping, so a multiplier above 1.0 yields an out-of-range alpha in the constructed color.

♻️ Proposed clamp

-    result[3] *= alpha_multiplier;
+    result[3] = (result[3] * alpha_multiplier).clamp(0.0, 1.0);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_interpolation.rs` at line 364, Update the
alpha handling in the color interpolation routine containing interpolated_alpha
and result[3] so the value is clamped again after applying alpha_multiplier.
Preserve the existing multiplier operation, then constrain the resulting alpha
to the valid [0.0, 1.0] range before constructing the color.

---

54-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit tests for the spec tables in this module.

carry_forward_missing_components, fixup_hues, and substitute_missing_components are pure functions over fixed-size arrays and encode the trickiest parts of css-color-4, yet this module has no #[cfg(test)] block while its sibling color_conversion.rs does. Table-driven tests over the analogous-component and hue-fixup cases would pin the behavior cheaply.

Want me to draft the test module?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_interpolation.rs` around lines 54 - 62,
Add a #[cfg(test)] module in color_interpolation.rs with table-driven unit tests
covering the pure functions carry_forward_missing_components, fixup_hues, and
substitute_missing_components. Include cases for analogous-component handling,
all-missing component groups, and the CSS Color 4 hue-fixup scenarios, asserting
each fixed-size array result against the specification tables.

---

217-219: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Avoid .unwrap() in an abort-on-panic FFI path.

The unwrap() is currently safe because every powerless-true arm has a hue index, but the invariant lives in a separate match. A future polar arm would turn this into a process abort via abort_on_panic.

♻️ Proposed defensive rewrite

-    if powerless {
-        missing[hue_index(target_type).unwrap()] = true;
-    }
+    if powerless {
+        if let Some(index) = hue_index(target_type) {
+            missing[index] = true;
+        }
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/color_interpolation.rs` around lines 217 - 219,
Update the powerless handling around target_type and hue_index to avoid calling
unwrap in this abort-on-panic FFI path. Validate or pattern-match the hue index
before assigning missing, and handle the impossible/no-hue case defensively
without panicking while preserving the existing behavior for hue-bearing polar
types.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.cpp (1)

429-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deduplicate the default Oklab interpolation-method construction. Both call sites build the same ColorInterpolationMethodStyleValue inline on every invocation (each one also allocating Rust FFI data), and both repeat the same static_cast<StyleValue const&> ternary dance.

• Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.cpp#L429-L432: replace the inline default with the shared helper and pass the resulting ref to interpolate_color_in_rust.
• Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.cpp#L460-L463: use the same helper for the absolutized path.

♻️ Proposed helper + call-site updates

Add near the other file-local helpers:

static ValueComparingNonnullRefPtr<StyleValue const> default_color_interpolation_method()
{
    return ColorInterpolationMethodStyleValue::create(RectangularColorSpace::Oklab);
}

to_color():

-    auto default_color_interpolation_method = ColorInterpolationMethodStyleValue::create(RectangularColorSpace::Oklab);
-    auto const& color_interpolation_method = color_interpolation_method_value()
-        ? *color_interpolation_method_value()
-        : static_cast<StyleValue const&>(*default_color_interpolation_method);
+    auto interpolation_method = color_interpolation_method_value()
+        ? ValueComparingNonnullRefPtr<StyleValue const> { *color_interpolation_method_value() }
+        : default_color_interpolation_method();
     auto style_value = interpolate_color_in_rust(
         *first_component().color,
         *second_component().color,
         normalized.second_percentage.as_fraction(),
         normalized.alpha_multiplier,
-        color_interpolation_method,
+        *interpolation_method,
         color_resolution_context);

absolutized():

-    auto default_color_interpolation_method = ColorInterpolationMethodStyleValue::create(RectangularColorSpace::Oklab);
-    auto const& color_interpolation_method = absolutized_color_interpolation_method
-        ? *absolutized_color_interpolation_method
-        : static_cast<StyleValue const&>(*default_color_interpolation_method);
+    auto interpolation_method = absolutized_color_interpolation_method
+        ? ValueComparingNonnullRefPtr<StyleValue const> { *absolutized_color_interpolation_method }
+        : default_color_interpolation_method();

(and pass *interpolation_method at the interpolate_color_in_rust call on L486.)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.cpp` around lines 429 -
432, Deduplicate default Oklab interpolation-method creation in
ColorMixStyleValue.cpp by adding a file-local
default_color_interpolation_method() helper returning the shared StyleValue
reference. Update both to_color() at lines 429-432 and absolutized() at lines
460-463 to use this helper and pass the resulting reference to
interpolate_color_in_rust, including the call at line 486.

Libraries/LibWeb/CSS/StyleValues/CalculatedStyleValue.cpp (1)

221-304: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Make CalcResolutionSnapshot non-copyable/non-movable.

It owns FFI storage released in the destructor and stores &length_resolution_context.value() into ffi_context, so any copy or move silently produces a dangling interior pointer plus a double release. All current uses are local named objects, but the type should enforce that.

♻️ Proposed guard

 struct CalcResolutionSnapshot {
+    AK_MAKE_NONCOPYABLE(CalcResolutionSnapshot);
+    AK_MAKE_NONMOVABLE(CalcResolutionSnapshot);
+
     CalcResolutionSnapshot(StyleValueFFI::CalcNode const* root, CalculationContext const& calculation_context, CalculationResolutionContext const& resolution_context)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/CalculatedStyleValue.cpp` around lines 221 -
304, Make CalcResolutionSnapshot explicitly non-copyable and non-movable by
deleting its copy constructor, copy assignment operator, move constructor, and
move assignment operator. Keep the existing destructor and constructor behavior
unchanged, ensuring local named instances remain usable while preventing unsafe
ownership or interior-pointer duplication.

Libraries/LibWeb/CSS/StyleValues/EasingStyleValue.cpp (1)

44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider hoisting the adopt/adopt_optional helpers into a shared header.

The same two lambdas are duplicated verbatim in Libraries/LibWeb/CSS/StyleValues/BasicShapeStyleValue.cpp (lines 59-68) and presumably in other FFI-data constructors. A shared inline helper (e.g. next to RustStyleValueHandle) would keep the retain/adopt contract in one place.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/EasingStyleValue.cpp` around lines 44 - 53,
Hoist the duplicated adopt and adopt_optional lambdas from the current
constructor and BasicShapeStyleValue into a shared inline helper near
RustStyleValueHandle. Update both call sites to use the shared helpers while
preserving their retain/adopt behavior and nullable handling, and reuse the
helpers in other matching FFI-data constructors where applicable.

Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.h (1)

32-51: 📐 Maintainability & Code Quality | 🔵 Trivial

Correct, but duplicates a retain-or-null helper seen elsewhere.

Parameter order into rust_style_value_create_color_mix matches the Rust signature. The local retain lambda here duplicates the same nullable-retain pattern used in AbstractImageStyleValue.cpp; see consolidated comment.

Also applies to: 66-69

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/ColorMixStyleValue.h` around lines 32 - 51,
Replace the local retain lambda in make_color_mix_data with the existing shared
nullable-retain helper used by AbstractImageStyleValue.cpp, preserving null
handling and argument order for rust_style_value_create_color_mix. Apply the
same deduplication to the related code at the constructor area around the
additionally noted lines.

Libraries/LibWeb/CSS/StyleValues/ConicGradientStyleValue.cpp (1)

33-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Extract a shared helper for the "retain + adopt" FFI child-value idiom.

The same three-step sequence — static_cast<StyleValueFFI::StyleValueData const*>(ptr) → StyleValueFFI::rust_style_value_retain(...) → StyleValue::adopt_rust_style_value_data(...) — is copy-pasted (with and without a null-check wrapper) across every subclass reconstructing a child value from FFI data. Centralizing this in one or two static helpers on StyleValue (nullable + non-null variants) would remove the duplication and reduce the risk of a call site omitting the retain call (dangling Rust data) or the null check (crash on an optional field) as this pattern keeps getting copied to new subclasses.

• Libraries/LibWeb/CSS/StyleValues/ConicGradientStyleValue.cpp#L33-L46: replace both inline lambdas with calls to a shared StyleValue::adopt_rust_optional_child(void const*) helper.
• Libraries/LibWeb/CSS/StyleValues/OpacityValueStyleValue.h#L38-L38: replace with a shared StyleValue::adopt_rust_child(void const*) (non-null) helper call.
• Libraries/LibWeb/CSS/StyleValues/FunctionStyleValue.h#L34-L34: replace with the same non-null helper.
• Libraries/LibWeb/CSS/StyleValues/SuperellipseStyleValue.h#L40-L40: replace with the same non-null helper.
• Libraries/LibWeb/CSS/StyleValues/RadialSizeStyleValue.h#L67-L74: replace both m_value_0/m_value_1 initializations with the nullable helper.
• Libraries/LibWeb/CSS/StyleValues/CounterDefinitionsStyleValue.cpp#L20-L21: replace the loop body's inline null-check/retain/adopt with the nullable helper.
• Libraries/LibWeb/CSS/StyleValues/StyleValueList.h#L74-L75: replace the loop body's retain/adopt with the non-null helper.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/ConicGradientStyleValue.cpp` around lines 33
- 46, Extract shared StyleValue helpers named adopt_rust_child(void const*) for
required FFI children and adopt_rust_optional_child(void const*) for nullable
children, preserving retain-before-adopt and null handling. Update
Libraries/LibWeb/CSS/StyleValues/ConicGradientStyleValue.cpp lines 33-46,
OpacityValueStyleValue.h line 38, FunctionStyleValue.h line 34,
SuperellipseStyleValue.h line 40, RadialSizeStyleValue.h lines 67-74,
CounterDefinitionsStyleValue.cpp lines 20-21, and StyleValueList.h lines 74-75
to use the appropriate helper; both ConicGradient lambdas use the nullable
helper, and each listed non-null or nullable site follows its indicated variant.

Tests/LibWeb/TestStyleValueEquality.cpp (1)

1308-1316: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract a helper for the repeated FfiAnimationContext literal.

The full FfiAnimationContext aggregate is duplicated verbatim across 7 test cases, differing only in allow_discrete (and, once, the transform reference box). Since this struct just gained new fields (current_color, has_length_resolution_context, length_resolution_context) in this PR, duplicating the literal means a future field addition can be silently zero/default-initialized at any site that isn't manually updated, since designated-initializer omissions don't fail to compile.

A small factory reduces that risk and the boilerplate:

♻️ Suggested helper

static StyleValueFFI::FfiAnimationContext make_animation_context(bool allow_discrete, bool has_transform_reference_box = false, double reference_box_width = 0, double reference_box_height = 0)
{
    return {
        .allow_discrete = allow_discrete,
        .current_color = nullptr,
        .has_length_resolution_context = false,
        .length_resolution_context = {},
        .has_transform_reference_box = has_transform_reference_box,
        .transform_reference_box_width = reference_box_width,
        .transform_reference_box_height = reference_box_height,
    };
}

Also applies to: 1333-1341, 1379-1387, 1428-1436, 2090-2098, 2389-2397, 2441-2449

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Tests/LibWeb/TestStyleValueEquality.cpp` around lines 1308 - 1316, Extract a
shared make_animation_context helper in the test file that initializes every
FfiAnimationContext field, accepting allow_discrete and optional transform
reference-box settings. Replace all seven duplicated FfiAnimationContext
literals with calls to this helper, preserving each test’s existing values.

Libraries/LibWeb/CSS/Rust/src/style_compute.rs (1)

1935-1941: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Doc comment doesn't cover COMPUTED_KIND_STYLE_VALUE. The replacement for that kind travels in computed_data, not value; worth a word so the FFI contract reads unambiguously.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/Rust/src/style_compute.rs` around lines 1935 - 1941,
Update the documentation for the computed-value struct fields, especially
computed_kind and value, to explicitly describe COMPUTED_KIND_STYLE_VALUE and
state that its replacement is stored in computed_data rather than value. Keep
the existing explanations for COMPUTED_KIND_UNCHANGED and the other kinds
accurate and make the FFI storage contract unambiguous.

Libraries/LibWeb/CSS/StyleValues/EasingStyleValue.cpp (1)

44-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider hoisting the retain/adopt/adopt_optional helpers into a shared header.

The identical trio is now duplicated across the migrated subclasses (e.g. BasicShapeStyleValue.cpp lines 25-27 and 59-68). A small shared inline helper next to StyleValue::adopt_rust_style_value_data would keep the retain/adopt contract in one place as more subclasses migrate.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/EasingStyleValue.cpp` around lines 44 - 53,
Move the duplicated retain/adopt helpers currently defined in
EasingStyleValue.cpp into a shared header alongside
StyleValue::adopt_rust_style_value_data, exposing reusable inline helpers for
required and optional values. Update EasingStyleValue and the other migrated
subclasses, including BasicShapeStyleValue, to use the shared helpers while
preserving their existing pointer and null-handling behavior.

Libraries/LibWeb/CSS/StyleValues/BasicShapeStyleValue.cpp (1)

88-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

path() round-trips through serialize/re-parse on every construction.

Rust stores the serialized instruction string, so every C++ facade rebuild re-runs SVG::AttributeParser::parse_path_data. That is a non-trivial cost for shapes adopted repeatedly during animation/cascade, and re-parsing is also a potential fidelity risk if serialization ever normalizes differently than the original input. Consider caching the parsed path per data pointer if this shows up in profiles.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Libraries/LibWeb/CSS/StyleValues/BasicShapeStyleValue.cpp` around lines 88 -
91, Cache the parsed path for each underlying shape data pointer in the case 6
branch of BasicShapeStyleValue construction, reusing the cached Path on repeated
facade creation instead of calling SVG::AttributeParser::parse_path_data each
time. Preserve the existing fill rule and path parsing behavior, and ensure the
cache lifetime safely tracks the referenced shape data.